Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Show root problem list for objects with problem and are part of dependency #1057

Open
wants to merge 2 commits into
base: dependencies
Choose a base branch
from

Conversation

raviks789
Copy link
Contributor

@raviks789 raviks789 commented Sep 17, 2024

ref #1050

blocked by: #1055
blocked by: Icinga/ipl-web#231

The previous relations didn't work as expected.
Now, filtering with `child.host.name` or
`parent.service.name` works fine.
@cla-bot cla-bot bot added the cla/signed CLA is signed by all contributors of a PR label Sep 17, 2024
@raviks789 raviks789 force-pushed the root-problem-list branch 10 times, most recently from bc06e1c to 3375e51 Compare September 19, 2024 13:14
@raviks789 raviks789 changed the title WIP Show root problem list for objects that have problem and are part of dependency Sep 19, 2024
@raviks789 raviks789 changed the title Show root problem list for objects that have problem and are part of dependency Show root problem list for objects with problem and are part of dependency Sep 19, 2024
@raviks789 raviks789 force-pushed the root-problem-list branch 5 times, most recently from 4351639 to e77c687 Compare September 23, 2024 07:52
@raviks789 raviks789 marked this pull request as ready for review September 23, 2024 12:19
Copy link
Member

@nilmerg nilmerg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rebase, #1055 is merged.

*
* @return ?BaseHtmlElement
*/
protected function createGroup(string $state)
protected function createGroup(string $state, bool $createLink = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop $createLink and replace it with $this->url !== null

abstract protected function getStateInt(string $state): int;
protected function getStateInt(string $state): int
{
return 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throw InvalidArgumentException instead

*
* @return ?BaseHtmlElement
*/
protected function createBadge(string $state)
protected function createBadge(string $state, bool $createLink = true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop $createLink and replace it with $this->url !== null


use Icinga\Module\Icingadb\Common\StateBadges;

class RedundancyGroupParentStateBadges extends StateBadges
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure why you named this RedundancyGroupParentStateBadges.

Can't it be just RedundancyGroupStateBadges?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since I just requested that RedundancyGroupStatistics should be renamed to ObjectsStatistics, these should be ObjectsStateBadges.

Comment on lines +13 to +18
return 'redundancy_group_parent';
}

protected function getPrefix(): string
{
return 'parents';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Groups have members.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, objects 😅


protected function createRootProblems(): ?array
{
$rootProblems = UnreachableParent::on($this->getDb(), $this->object)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

return early if the object is reachable.

use ipl\Html\ValidHtml;
use ipl\Html\HtmlString;

class RedundancyGroupStatistics extends ObjectStatistics
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename this to ObjectsStatistics. There is not much which makes this redundancy group specific.

It's just not specific to hosts and services anymore. We will benefit from a generalized implementation somewhere else ;)

Comment on lines +26 to +33
->addSlice($this->summary->parents_ok, ['class' => 'slice-state-ok'])
->addSlice($this->summary->parents_warning_handled, ['class' => 'slice-state-warning-handled'])
->addSlice($this->summary->parents_warning_unhandled, ['class' => 'slice-state-warning'])
->addSlice($this->summary->parents_problem_handled, ['class' => 'slice-state-critical-handled'])
->addSlice($this->summary->parents_problem_unhandled, ['class' => 'slice-state-critical'])
->addSlice($this->summary->parents_unknown_handled, ['class' => 'slice-state-unknown-handled'])
->addSlice($this->summary->parents_unknown_unhandled, ['class' => 'slice-state-unknown'])
->addSlice($this->summary->parents_pending, ['class' => 'slice-state-pending']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace parents_ with objects_

Comment on lines +65 to +117
$filter = Filter::equal('id', $this->item->id);
$relations = [
'from',
'from.to.host',
'from.to.host.state',
'from.to.service',
'from.to.service.state'
];

$summary = RedundancyGroupParentStateSummary::on($this->getDb())
->with($relations)
->filter($filter);

$members = RedundancyGroup::on($this->getDb())
->columns([
'id' => 'id',
'parent_output' => new Expression(
'CASE WHEN redundancy_group_from_to_host_state.output IS NULL'
. ' THEN redundancy_group_from_to_service_state.output'
. ' ELSE redundancy_group_from_to_host_state.output END'
),
'parent_long_output' => new Expression(
'CASE WHEN redundancy_group_from_to_host_state.long_output IS NULL'
. ' THEN redundancy_group_from_to_service_state.long_output'
. ' ELSE redundancy_group_from_to_host_state.long_output END'
),
'parent_checkcommand_name' => new Expression(
'CASE WHEN redundancy_group_from_to_host.checkcommand_name IS NULL'
. ' THEN redundancy_group_from_to_service.checkcommand_name'
. ' ELSE redundancy_group_from_to_host.checkcommand_name END'
),
'parent_last_state_change' => new Expression(
'CASE WHEN redundancy_group_from_to_host_state.last_state_change IS NULL'
. ' THEN redundancy_group_from_to_service_state.last_state_change'
. ' ELSE redundancy_group_from_to_host_state.last_state_change END'
),
'parent_severity' => new Expression(
'CASE WHEN redundancy_group_from_to_host_state.severity IS NULL'
. ' THEN redundancy_group_from_to_service_state.severity'
. ' ELSE redundancy_group_from_to_host_state.severity END'
)
])
->with($relations)
->filter($filter)
->orderBy([
'parent_severity',
'parent_last_state_change',
], 'DESC');

$this->applyRestrictions($members);

/** @var RedundancyGroup $data */
$data = $members->first();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite like that this is evaluated per group result.

Keep it as it is for now, but update it so that it matches the changes made to the statistics and badges implementation.

I have an alternative implementation in mind, but that's now far to complex for this change and so will be implemented separately. Prior approval of this change, I will request to remove this to not show any badges or output for groups. Until then, it's kept to test the statistics and badges implementation changes.

Comment on lines +132 to +141
$verb = 'has';
} else {
$verb = 'is';
}

$title->addHtml(Html::sprintf(
t('%s %s %s', '<hostname> has/is <state-text>'),
$this->createSubject(),
$verb,
Html::tag('span', ['class' => 'state-text'], $this->state->failed ? 'Failed' : 'OK')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Translatable strings should never be concatenated. Instead, the text to translate should always be complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla/signed CLA is signed by all contributors of a PR enhancement New feature or improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show a list of root problems in the host/service detail if it has a problem and is part of a dependency
2 participants